-
Notifications
You must be signed in to change notification settings - Fork 7.3k
tui: move session logging off the main thread #8979
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
9dd2f7e to
72eb131
Compare
72eb131 to
ddfa35d
Compare
|
I have read the CLA Document and I hereby sign the CLA |
|
recheck |
|
@jif-oai Could you take a look at this? |
|
@codex review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ddfa35d4f8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Thanks for working on this, and for writing up the motivation. I agree with the direction of moving TUI session logging off the UI thread. Background plus buffered writes is exactly the right shape of solution here. I am more concerned about this part though:
Those logs are not only for post-mortems, we use them actively to debug live Codex CLI sessions while they are running. Losing durability until exit would make a class of "Codex hung" or "Codex misbehaved" issues much harder to diagnose. One thing to flag on process. I am currently in the middle of another change to our TUI logging pipeline around consistent session id propagation, and there is some overlap with what this PR touches. Because of that I am probably going to hold off on reviewing or merging this until that work lands so we do not end up doing the same surgery twice in slightly different ways. If this change is fixing a concrete, user visible problem, for example the input lag mentioned in #8976, it would really help to have a bit more diagnosis in the issue itself that shows session logging on the UI thread is the root cause. Even something like timings, traces, or a small repro would let us validate that we are fixing the right thing and not just a plausible theory. Happy to circle back once the logging work is in, and I appreciate you digging into this. |
|
One more thing to add. This part of the TUI stack is still under documented and under tested for how critical it is. Session logging sits right on the boundary between the UI thread, IO, and shutdown, so small changes can easily introduce subtle correctness or performance issues. Even if we keep most of this approach, I think it would be good to use this PR to add at least some basic coverage or instrumentation around things like flush on exit and dropped events so we can change this code with more confidence going forward. |
|
@joshka-oai Thanks for the feedback. I agree on the durability concerns and the need for coverage. I can keep the background writer but add a periodic flush (e.g., 250–500ms) so logs remain usable for live debugging, while still avoiding synchronous I/O on the UI thread. We can also keep the synchronous flush-on-exit to guarantee I’ll add tests around flush-on-exit (ensuring the line is actually present on Let me know if a specific flush cadence or hook (idle tick vs fixed interval) is preferred, so I can update the PR accordingly. |
|
The main thing I want to reiterate is that if this change is intended to fix a concrete, user-visible issue, for example the input lag described in #8976, we really need more diagnosis captured in the issue itself that demonstrates session logging on the UI thread is the actual root cause. That does not need to be exhaustive, but even basic evidence such as timings, traces, or a minimal repro would help us validate that we are fixing the right problem rather than a plausible theory. Without that, it is hard to judge whether this change is correctly targeted. Separately, looking at the implementation as it stands, it introduces a significant amount of additional, undocumented complexity. This includes deeply indented control flow, an extra abstraction layer between tracing and output, and mpsc channels. Before this would be in a state we are comfortable landing, that complexity needs to be substantially reduced and clearly justified. |
Summary
tuiandtui2.Motivation
Fix input lag under high disk usage by eliminating synchronous disk writes on the UI thread.
Testing
cargo test -p codex-tuicargo test -p codex-tui2Issue